Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unsafe from ReadToString #2384

Merged
merged 3 commits into from
Apr 21, 2020
Merged

Remove unsafe from ReadToString #2384

merged 3 commits into from
Apr 21, 2020

Conversation

goffrie
Copy link
Contributor

@goffrie goffrie commented Apr 8, 2020

We can do everything needed with only safe code.

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the documentation of AsyncReadExt::read_to_string, the buffer should be left unchanged when the data is invalid utf-8, but this implementation empties the string in this case. To be fair, the previous implementation also emptied the string, but it'd still be nice to fix that.

You can test this behavior by creating tokio/tests/read_to_string.rs with these contents:

use tokio::io::AsyncReadExt;

#[tokio::test]
async fn to_string_does_not_truncate() {
    let data = vec![0xff, 0xff, 0xff];

    let mut s = "abc".to_string();

    match AsyncReadExt::read_to_string(&mut data.as_slice(), &mut s).await {
        Ok(len) => panic!("Should fail: {} bytes.", len),
        Err(err) if err.to_string() == "stream did not contain valid UTF-8" => {},
        Err(err) => panic!("Fail: {}.", err),
    }

    assert_eq!(s, "abc");
}

#[tokio::test]
async fn to_string_appends() {
    let data = b"def".to_vec();

    let mut s = "abc".to_string();

    let len = AsyncReadExt::read_to_string(&mut data.as_slice(), &mut s).await.unwrap();

    assert_eq!(len, 3);
    assert_eq!(s, "abcdef");
}

I've also added a test in the case when the provided buffer is non-empty, but the data is valid utf-8. This test does not currently fail, but we might as well add it now that we're adding tests.

The CI failure is due to the unused import.

goffrie and others added 2 commits April 8, 2020 13:33
We can do everything needed with only safe code.
@goffrie
Copy link
Contributor Author

goffrie commented Apr 8, 2020

Thanks, good point. I've adjusted the implementation to try to put the buffer back on error. This doesn't work if the reader gets dropped, but fixing that doesn't really seem worth it.

@@ -121,7 +121,7 @@ default-features = false
optional = true

[dev-dependencies]
tokio-test = { version = "0.2.0" }
tokio-test = { version = "0.2.0", path = "../tokio-test" }
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if this is kosher, but a new version of tokio-test with the read_error mock hasn't been released to crates.io yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be ok to keep path dev-deps. What would be the reason to avoid them?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't think of any reason, just wondering why it isn't already this way.

@jonhoo jonhoo added A-tokio Area: The main tokio crate C-maintenance Category: PRs that clean code up or issues documenting cleanup. M-io Module: tokio/io S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 17, 2020
@kleimkuhler
Copy link
Contributor

@goffrie You'll want to rebase this off master to fix the FreeBSD check but this looks good otherwise

Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had another read-through, and everything looks good!

@Darksonn Darksonn merged commit 2da15b5 into tokio-rs:master Apr 21, 2020
@taiki-e taiki-e removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 21, 2020
@goffrie goffrie deleted the patch-1 branch April 21, 2020 19:23
Darksonn added a commit to Darksonn/tokio that referenced this pull request May 16, 2020
The changes in this PR are inspired by tokio-rs#2384.

Fixes: tokio-rs#2532
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-maintenance Category: PRs that clean code up or issues documenting cleanup. M-io Module: tokio/io
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants